Skip to content

session, plan, exec, python, connect: Dataset.observe and Observation (P3-T084, PR-51) - #756

Open
techgopal wants to merge 23 commits into
mainfrom
p3/s4-t084-observe
Open

techgopal wants to merge 23 commits into
mainfrom
p3/s4-t084-observe

Conversation

@techgopal

Copy link
Copy Markdown
Contributor

P3-T084 / PR-51: Dataset.observe and Observation (embedded Rust, Python, Spark Connect ObservedMetrics).

What

  • PlanNode::CollectMetrics plus its analysis (INVALID_OBSERVED_METRICS.*, DUPLICATED_METRICS_NAME, in Spark 4.2 order); a pass-through operator that folds mergeable partition-local aggregate state (charged to the memory pool) and delivers it at end of input; plan-codec feature observe (fields 4 and 5 are plan data: instance and limit cap).
  • Session: DataFrame::observe, Observation (first action wins, get blocks); all actions complete the observation, a plan that observes skips the result cache. A LIMIT above the node caps the count as in Spark.
  • Python krishiv.sql.Observation / DataFrame.observe; Connect CollectMetrics translation and observed_metrics after a successful action.
  • Ground truth: real Spark 4.2.0 runs, 72 classic and 76 Connect cases in tests/fixtures/spark-4.2.0/observe-goldens.json; the runner asserts 63 classic cases and 7 Connect legs. Gaps are explicit in known-differences.md and pinned in KNOWN_GAPS (follow-up P3-T084c).
  • Design: design/P3-T084-observe.md, research research/t084-observe-semantics.md (krishiv-internal).

Public surface for the owner: krishiv_session::Observation (new, named, name, get, get_timeout), DataFrame::observe; Python Observation, DataFrame.observe; PhysNode::CollectMetrics fields instance, limit. Connect-only items (ObservedSink, with_observed_sink, observe_named, QueryContext::observed, PhysNode::measure) are #[doc(hidden)]. Snapshots regenerated.

Review: Opus, CHANGES_REQUIRED (7 blockers) then APPROVE after fixes: ops/reviews/2026-11-14-sonnet-4-p3-t084-observe-diff.md in krishiv-internal. Squash-merge please (the branch history is not bisectable: the commit hook runs workspace clippy).

Known gaps (P3-T084c): sort+limit counts, ANSI sum overflow, subquery in a metric, where(false) pruning, streaming refusal, struct/map/interval metrics over Connect, per-task limit budget when distributed.

…nerator (P3-T084)

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Dataset.observe as a logical node: rows pass through, the metrics resolve
as a global aggregate over the input, and Spark 4.2.0's CheckAnalysis
classes apply (INVALID_OBSERVED_METRICS.*, DUPLICATED_METRICS_NAME across
different relations, UNSUPPORTED_GENERATOR.OUTSIDE_SELECT).

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…l node and codec feature (P3-T084 unit 3)

PhysNode::CollectMetrics is plain data; the metrics lower through the
global aggregate planner. The operator folds each batch into a
partition-local partial aggregate state and forwards it unchanged. The
optimizer keeps filters and limits above the node and keeps the metrics'
inputs. Codec feature 'observe' with goldens; LIMIT 0 plans no node.
The query context collects driver states and finalizes them (last
instance of a name wins).

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Observation handle (get blocks, first action wins, completes on failure
and cancellation), delivery from the query context when the result stream
ends, KRISHIV_OBSERVATION_REUSED and KRISHIV_NO_OBSERVE_BEFORE_GET, and
end-to-end tests over the Spark 4.2.0 goldens, parallelism exactness and
cancellation.

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…etrics (P3-T084 units 5-6)

Python: krishiv._native.Observation, DataFrame.observe and
krishiv.sql.Observation mirroring pyspark 4.2 (get is a blocking property
returning a dict keyed by metric column names; REUSE_OBSERVATION,
NO_OBSERVE_BEFORE_GET, VALUE_NOT_NON_EMPTY_STR, NOT_EXPECTED_TYPE,
CANNOT_BE_EMPTY).

Connect: CollectMetrics translates to PlanNode::CollectMetrics; the server
sends ObservedMetrics (name, values, keys, plan_id) after the metrics
response of a successful action, none for a failed one. Session gets
ObservedSink for handle-less delivery. Coverage inventory updated.

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…, limit cap, pool-charged state (P3-T084 review fixes)

One delivery hook (run_on_query_logged) completes observations and Connect sinks for
reads, writes, table writes, collect_with_metrics and profiles; a plan that observes is
not served from the result cache. CollectMetrics carries a planner-assigned instance
(codec field 4) instead of a process counter, and a limit (field 5) set from a LIMIT above
so counts are rows taken, as Spark's. The operator charges its state to the driver's
consumer. A scalar subquery in a metric is KRISHIV_UNSUPPORTED_FEATURE. A failed
finalization is what Observation.get returns. Connect sends all names in one response.
public-api snapshots updated; internal items are doc(hidden).

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…PS, action and memory tests (P3-T084 review fixes)

observe_goldens.rs runs every case of observe-goldens.json or lists it as client-only or
a known gap that must still fail (sort+limit, ANSI sum overflow, scalar subquery). The
Connect leg runs the flows_* and Connect-different cases through the server. Gaps are in
docs/concepts/known-differences.md (follow-up P3-T084c).

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Nov 14, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 14, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-15T01:00:48.474511Z 1bf3315 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7661380303

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/krishiv-exec/src/context.rs Resolved
@codspeed

codspeed Bot commented Nov 14, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 28 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 224 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing p3/s4-t084-observe (76daa23) with main (6f40584)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…changelog fragment (#756)

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…s, memory charge, per-observation errors, temp views)

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@codecov

codecov Bot commented Nov 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
python/krishiv/sql/observation.py 90.9% 4 Missing ⚠️
python/krishiv/sql/dataframe.py 87.5% 2 Missing ⚠️
crates/krishiv-session/src/catalog.rs 97.1% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…lt's lease; refresh python public-api snapshot

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@techgopal

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61891b7200

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…hon UDF in an observed metric (P3-T084 review)

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@techgopal

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 71e34a5d47

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…-size gate, P3-T084)

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@techgopal

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f2c8f6e908

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/krishiv-session/src/dataframe.rs Resolved
…observation handles (P3-T084 review)

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@techgopal

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61af3039c4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/krishiv-exec/src/context.rs Resolved
…ask observations reach the query (P3-T084 review)

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@techgopal

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: aaa6ca222a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>

# Conflicts:
#	crates/krishiv-exec/src/physical/mod.rs
#	crates/krishiv-exec/src/planner/build.rs
#	crates/krishiv-optimizer/src/limit.rs
#	crates/krishiv-plan-codec/src/decode.rs
#	crates/krishiv-plan-codec/src/encode.rs
#	crates/krishiv-plan-codec/src/features.rs
#	crates/krishiv-plan-codec/src/proto/krishiv.plan.v1.pb.rs
#	crates/krishiv-plan-codec/tests/features.rs
#	crates/krishiv-plan/src/analyzer/resolve.rs
#	crates/krishiv-plan/src/encode.rs
#	crates/krishiv-plan/src/plan.rs
#	crates/krishiv-session/src/catalog.rs
#	crates/krishiv-session/src/result_cache.rs
#	crates/krishiv-session/src/session/env.rs
#	proto/krishiv/plan/v1/plan.proto
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@techgopal

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 1bf3315db1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@techgopal

Copy link
Copy Markdown
Contributor Author

main merged in for the CI disk fix (#776); lane: git pull before your next push

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant